Skip to content

Conversation

@MasterPtato
Copy link
Contributor

No description provided.

Copy link
Contributor Author

MasterPtato commented Jan 13, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Jan 13, 2026

Pull Request Review: fix(otel): enrich http traces

Overview

This PR adds OpenTelemetry HTTP semantic attributes to HTTP traces in both the API builder middleware and guard proxy service. The changes enhance observability by adding standard HTTP metadata to trace spans.

Code Quality & Best Practices

Positive Aspects:

  • Clean, minimal changes that focus on a specific improvement
  • Consistent pattern applied across both middleware and proxy service
  • Proper use of OpenTelemetry span attributes via set_attribute()

Issues & Suggestions:

  1. ⚠️ Inconsistent Attribute Names - Semantic Convention Mismatch

    The PR uses a mix of attribute naming conventions:

    • http.request.method ✅ (correct for newer semantic conventions)
    • http.response.status_code ✅ (correct)
    • http.route ✅ (correct)
    • http.path ❌ (should be url.path or http.target per OpenTelemetry semantic conventions)

    Location:

    • engine/packages/api-builder/src/middleware.rs:71
    • engine/packages/guard-core/src/proxy_service.rs:2039

    Recommendation: According to OpenTelemetry semantic conventions (v1.21+), HTTP attributes should use:

    • http.request.method for the HTTP method ✅
    • http.response.status_code for status code ✅
    • http.route for the matched route pattern ✅
    • url.path for the URL path (not http.path)

    Change http.path to url.path for semantic convention compliance.

  2. Minor: Redundant String Conversion in middleware.rs

    Location: engine/packages/api-builder/src/middleware.rs:70-71

    req_span.set_attribute("http.request.method", req.method().to_string());
    req_span.set_attribute("http.path", req.uri().to_string());

    These conversions happen right after the span is created with the same values already in the span fields (lines 65-66). While adding them as attributes is correct for OTel exporters, the duplication is worth noting.

    Not a blocker, but consider if you want the span fields and attributes to match exactly or if they serve different purposes.

  3. Code Style: Variable Assignment Pattern

    Location: engine/packages/guard-core/src/proxy_service.rs:2013-2016

    The refactoring from chained .record() calls to storing current_span in a variable is good for readability and enables the subsequent set_attribute() calls. This follows good Rust practices. ✅

Potential Bugs or Issues

No critical bugs identified. The changes are straightforward additions that shouldn't introduce runtime issues.

However:

  • The http.path vs url.path naming could cause confusion when correlating with other services that follow OpenTelemetry conventions correctly
  • Observability tools may not recognize http.path as a standard attribute

Performance Considerations

Impact: Minimal

  • String allocations via .to_string() are added on the hot path (every HTTP request)
  • These are small allocations (method strings, URI strings) and the observability benefit outweighs the cost
  • The attributes are only set once per request, not in loops
  • Performance impact is acceptable for the observability gains

Security Concerns

No security issues identified.

  • URI and method information is already being logged (lines 65-66 in middleware.rs, line 2051+ in proxy_service.rs)
  • No sensitive data is being added to traces that wasn't already present in logs
  • The attributes follow standard patterns for HTTP tracing

Test Coverage

Observation: No tests are included in this PR.

Recommendation: While the changes are low-risk, consider adding integration tests that verify:

  1. The OpenTelemetry attributes are correctly set on spans
  2. The attributes contain expected values for various HTTP requests
  3. The http.route attribute is properly set when using the MatchedPath extractor

This is not a blocking issue for such a straightforward change, but would improve confidence.

Additional Notes

  1. Dependency Addition: The PR correctly adds tracing-opentelemetry to guard-core/Cargo.toml as a workspace dependency. ✅

  2. Import Addition: The import of MatchedPath extractor in middleware.rs is correct and necessary for the http.route attribute. ✅

  3. Consistency: The same pattern is applied in both files, which is good for maintainability. ✅

Summary

This is a solid PR that improves observability with minimal risk. The main issue is the use of http.path instead of the standard url.path attribute name.

Recommended Actions:

  1. Required: Change http.path to url.path in both files for OpenTelemetry semantic convention compliance
  2. Optional: Add basic integration tests for trace attribute verification
  3. Optional: Document why span fields and attributes contain overlapping information (if intentional)

After the http.pathurl.path fix, this PR should be ready to merge.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 13, 2026

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3878

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3878

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3878

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3878

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3878

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3878

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3878

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3878

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3878

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3878

commit: 116ea58

@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch from c6bf57a to 5f2275d Compare January 13, 2026 20:07
@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch from 5f2275d to a35f0d9 Compare January 14, 2026 02:05
@MasterPtato MasterPtato force-pushed the 01-12-fix_guard_clean_up_guard branch from c079c96 to 9e8cf3d Compare January 14, 2026 02:05
@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch from a35f0d9 to e88b75c Compare January 14, 2026 02:53
@MasterPtato MasterPtato force-pushed the 01-12-fix_guard_clean_up_guard branch from 9e8cf3d to 3008b28 Compare January 14, 2026 02:53
@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch from e88b75c to 74129c7 Compare January 14, 2026 03:06
@MasterPtato MasterPtato force-pushed the 01-12-fix_guard_clean_up_guard branch from 3008b28 to 70900c0 Compare January 14, 2026 03:06
@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch from 74129c7 to c3c6740 Compare January 14, 2026 19:47
@MasterPtato MasterPtato force-pushed the 01-12-fix_guard_clean_up_guard branch from 70900c0 to 5f50433 Compare January 14, 2026 19:47
@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch from c3c6740 to cdb8231 Compare January 14, 2026 20:01
@MasterPtato MasterPtato force-pushed the 01-12-fix_guard_clean_up_guard branch from 5f50433 to d4e5e20 Compare January 14, 2026 20:01
@MasterPtato MasterPtato force-pushed the 01-12-fix_guard_clean_up_guard branch from d4e5e20 to f831199 Compare January 14, 2026 22:45
@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch from cdb8231 to 1353291 Compare January 14, 2026 22:45
@MasterPtato MasterPtato force-pushed the 01-12-fix_guard_clean_up_guard branch from f831199 to c90f0b8 Compare January 14, 2026 22:47
@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch 2 times, most recently from 921ccb3 to e8bbcf8 Compare January 14, 2026 22:52
@MasterPtato MasterPtato force-pushed the 01-12-fix_guard_clean_up_guard branch from c90f0b8 to 9d72300 Compare January 14, 2026 22:52
@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch from e8bbcf8 to d8a0135 Compare January 14, 2026 23:02
@MasterPtato MasterPtato force-pushed the 01-12-fix_guard_clean_up_guard branch from 875a7a4 to 920b1d9 Compare January 14, 2026 23:07
@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch from d8a0135 to 116ea58 Compare January 14, 2026 23:07
@MasterPtato MasterPtato force-pushed the 01-13-fix_otel_enrich_http_traces branch from 116ea58 to 708a38f Compare January 14, 2026 23:39
@MasterPtato MasterPtato force-pushed the 01-12-fix_guard_clean_up_guard branch from 920b1d9 to 4a1bc29 Compare January 14, 2026 23:39
@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 14, 2026

Merge activity

  • Jan 14, 11:40 PM UTC: MasterPtato added this pull request to the Graphite merge queue.
  • Jan 14, 11:41 PM UTC: CI is running for this pull request on a draft pull request (#3908) due to your merge queue CI optimization settings.
  • Jan 14, 11:42 PM UTC: Merged by the Graphite merge queue via draft PR: #3908.

graphite-app bot pushed a commit that referenced this pull request Jan 14, 2026
@graphite-app graphite-app bot closed this Jan 14, 2026
@graphite-app graphite-app bot deleted the 01-13-fix_otel_enrich_http_traces branch January 14, 2026 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants